-
Notifications
You must be signed in to change notification settings - Fork 14.3k
Msf::Module::UUID#generate_uuid: Replace Rex::Text with SecureRandom.uuid #20170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
The UUID that isn't a UUID has always confused me; I'm happy to give this a go 🤞
I'm hoping this is just treated as an opaque value so it shouldn't cause any issues - but I'll run the test suite behind the scenes and see what happens 👍 |
Thanks for the PR! Just spent a few more cycles on this - including accidentally testing on Ruby 2.7.2 and things were much slower for me:
But thankfully Ruby 3.4.1 is faster these days 😄
I think using a real UUID will cause issues with the current database constraints that are implemented for task objects: metasploit-framework/db/schema.rb Line 621 in b251fc1
We could likely remove/change this constraints etc - but I don't know if that'll cause any issues for external tools using this value with similar limits/assumptions applied etc. Looking at a slightly different/alternative approaches - what are your thoughts on keeping the same generation logic for now; but using a faster way of generating the same values that Metasploit currently uses 🤔 # frozen_string_literal: true
require 'benchmark'
require 'securerandom'
require 'rex/text'
module FasterUuid
UUID_CHARS = [*('a'..'z'), *('0' .. '9')].freeze
private_constant :UUID_CHARS
def self.fast_uuid
UUID_CHARS.sample(8).join
end
end
n = 100_000
Benchmark.bm(20) do |bm|
bm.report('Rex::Text.rand_text') do
n.times { uuid = Rex::Text.rand_text_alphanumeric(8).downcase }
end
bm.report('SecureRandom.uuid') do
n.times { uuid = SecureRandom.uuid }
end
bm.report('FasterUuid.fast_uuid') do
n.times { uuid = FasterUuid.fast_uuid }
end
end Ruby 3.4.1
Using # frozen_string_literal: true
require 'securerandom'
require 'rex/text'
require 'benchmark/ips'
module FasterUuid
UUID_CHARS = [*('a'..'z'), *('0' .. '9')].freeze
private_constant :UUID_CHARS
def self.fast_uuid
UUID_CHARS.sample(8).join
end
end
Benchmark.ips do |bm|
bm.report('Rex::Text.rand_text') do
Rex::Text.rand_text_alphanumeric(8).downcase
end
bm.report('FasterUuid::fast_uuid') do
FasterUuid::fast_uuid
end
bm.report('SecureRandom.uuid') do
SecureRandom.uuid
end
bm.compare!
end Ruby 2.7.2 $ bundle exec ruby ./speed.rb
ruby 2.7.2p137 (2020-10-01 revision 5445e04352) [x86_64-darwin20]
Warming up --------------------------------------
Rex::Text.rand_text 4.985k i/100ms
FasterUuid::fast_uuid
98.877k i/100ms
SecureRandom.uuid 903.000 i/100ms
Calculating -------------------------------------
Rex::Text.rand_text 48.631k (± 4.0%) i/s (20.56 μs/i) - 244.265k in 5.031811s
FasterUuid::fast_uuid
793.478k (±12.5%) i/s (1.26 μs/i) - 3.955M in 5.066235s
SecureRandom.uuid 9.807k (±18.5%) i/s (101.96 μs/i) - 46.956k in 5.048430s
Comparison:
FasterUuid::fast_uuid: 793478.4 i/s
Rex::Text.rand_text: 48631.2 i/s - 16.32x slower
SecureRandom.uuid: 9807.3 i/s - 80.91x slower
Ruby 3.4.1
Or making the value entirely lazy might work too, and we just generate the value when it's accessed? 🤔 diff --git a/lib/msf/core/module.rb b/lib/msf/core/module.rb
index ff455d4716..aed1b01b17 100644
--- a/lib/msf/core/module.rb
+++ b/lib/msf/core/module.rb
@@ -113,7 +113,6 @@ module Msf
@module_info_copy = info.dup
self.module_info = info
- generate_uuid
set_defaults
diff --git a/lib/msf/core/module/uuid.rb b/lib/msf/core/module/uuid.rb
index 5b3299d3df..523164c0bf 100644
--- a/lib/msf/core/module/uuid.rb
+++ b/lib/msf/core/module/uuid.rb
@@ -5,9 +5,10 @@ module Msf::Module::UUID
# Attributes
#
- # @!attribute [r] uuid
- # A unique identifier for this module instance
- attr_reader :uuid
+ # @return [String] A unique identifier for this module instance
+ def uuid
+ @uuid ||= generate_uuid
+ end
protected
@@ -18,12 +19,8 @@ module Msf::Module::UUID
# @!attribute [w] uuid
attr_writer :uuid
-
- #
- # Instance Methods
- #
-
def generate_uuid
- self.uuid = Rex::Text.rand_text_alphanumeric(8).downcase
+ @uuid = Rex::Text.rand_text_alphanumeric(8).downcase
end
end Potentially combining both approaches could work, i.e. faster 'uuid' generation, as well as making it lazy could work - but I'm not strongly opinionated; there might alternative options too - just let me know if you've any preferences on next steps 💯 |
Module UUIDs were added in commit 9277f06 titled "Store a uuid for each module, track this in sessions" in 2011. Module UUIDs differ between Metasploit runs, as they are dynamically generated at runtime using
Msf::Module::UUID#generate_uuid
.This method was authored in 2011 when Metasploit contained far fewer modules. Now this method is called approximately 27 thousand (!) times during startup. Due to the ever-increasing number of modules, this behaviour will cause the startup time to grow.
The UUIDs are generated with
Rex::Text.rand_text_alphanumeric(8).downcase
, which does not generate standard UUIDs.metasploit-framework/lib/msf/core/module/uuid.rb
Line 27 in 57032a3
The generated UUID is 8 characters in length (64bit). Most of the key space is wasted, as only 36 values (a-z and 0-9) of 256 are used.
Generating collisions is unlikely, and the impact to startup time is minimal, but the computation is needlessly expensive and wasteful:
rand_text_alphanumeric
returns mixed-case alphanumic characters, but we ultimately discard uppercaseA-Z
rand_text_alphanumeric
constructs an array, then callsRex::Text#rand_base
,[1] which:All we really want is a unique value that is unlikely to cause collisions. If UUID collisions or speed were an issue we could pre-compute values (which has the added benefit of consistency between Metasploit runs).
Instead, this PR replaces
Rex::Text.rand_text_alphanumeric
withSecureRandom.uuid
which is significantly faster (almost 10x 🚀):Benchmarked on a system with 2 cores and 4GB RAM:
This does not introduce an extra dependency as we already use SecureRandom in Framework:
Note: I'm not sure what effect this change will have on Metasploit Pro, if any.
Note: Maybe there is a reason we only want UUIDs to be only 8 characters? If so, we could simply truncate the generated UUID. This would still be much faster than using
Rex::Text
.[1] https://github.com/rapid7/rex-text/blob/0d30d394c4378dbaddf7b489f0d22c2db4024ec7/lib/rex/text/rand.rb#L110-L118
[2] https://github.com/rapid7/rex-text/blob/0d30d394c4378dbaddf7b489f0d22c2db4024ec7/lib/rex/text/rand.rb#L144-L151